Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FM refactoring and compile times reduction #163

Merged
merged 23 commits into from
Oct 4, 2023
Merged

FM refactoring and compile times reduction #163

merged 23 commits into from
Oct 4, 2023

Conversation

N-Maas
Copy link
Collaborator

@N-Maas N-Maas commented Sep 28, 2023

  • Refactors localized_kway_fm_core to reduce compile times and remove code that is not used by our configurations (also introduces a small optimization for graphs)
  • Includes several refactoring with the overall goal of reducing compile times. Notably, only valid combinations of graph data structure / gain type are now instantiated
  • Includes a few small fixes for previous changes (printing the parameters for unconstrained refinement, fixing some warnings)

Clean build times are ~1.5 times faster on my machine while the compilation time for the slowest files is reduced by a factor of 2-3.

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #163 (412ee65) into master (bf24f75) will increase coverage by 0.89%.
The diff coverage is 78.26%.

❗ Current head 412ee65 differs from pull request most recent head f7fe3c6. Consider uploading reports for the commit f7fe3c6 to get more accurate results

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
+ Coverage   78.32%   79.21%   +0.89%     
==========================================
  Files         201      203       +2     
  Lines       19530    19736     +206     
  Branches     7995     7988       -7     
==========================================
+ Hits        15296    15634     +338     
+ Misses       4234     4102     -132     
Files Coverage Δ
mt-kahypar/io/sql_plottools_serializer.cpp 92.56% <ø> (ø)
...ar/partition/coarsening/multilevel_uncoarsener.cpp 62.18% <100.00%> (+0.64%) ⬆️
...ahypar/partition/coarsening/nlevel_uncoarsener.cpp 78.21% <100.00%> (+0.21%) ⬆️
...-kahypar/partition/coarsening/nlevel_uncoarsener.h 100.00% <ø> (ø)
.../initial_partitioning/greedy_initial_partitioner.h 100.00% <100.00%> (ø)
...partitioning/initial_partitioning_data_container.h 93.20% <ø> (ø)
.../initial_partitioning/pool_initial_partitioner.cpp 87.09% <ø> (ø)
...-kahypar/partition/refinement/flows/flow_refiner.h 0.00% <ø> (ø)
...partition/refinement/flows/parallel_construction.h 100.00% <ø> (ø)
...t-kahypar/partition/refinement/flows/scheduler.cpp 75.42% <100.00%> (ø)
... and 24 more

... and 59 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@larsgottesbueren larsgottesbueren self-requested a review October 2, 2023 09:45
Copy link
Member

@larsgottesbueren larsgottesbueren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only minor comments.

@@ -45,12 +45,12 @@

namespace mt_kahypar {

template<typename TypeTraits, typename GainTypes>
template<typename CombinedTraits>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change. Can we rethink the name CombinedTraits?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure. Any suggestions? Maybe GraphAndGainTypes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about also removing the FM stats? We're way past using them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea 👍

using KWayPriorityQueue = kahypar::ds::KWayPriorityQueue<HypernodeID, Gain, std::numeric_limits<Gain>, false>;
using ThreadLocalKWayPriorityQueue = tbb::enumerable_thread_specific<KWayPriorityQueue>;

using ThreadLocalFastResetFlagArray = tbb::enumerable_thread_specific<kahypar::ds::FastResetFlagArray<> >;

using InitialPartitionerFactory = kahypar::meta::Factory<InitialPartitioningAlgorithm,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the best policy is, but let's discuss. Factories close to the definitions or factories in a dedicated factory header? We should try to be consistent, i.e., the refiner and coarsener factories are in a dedicated header.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I guess it would make sense to move InitialPartitionerFactory also to factories.h

using DeltaPartitionedHypergraph = typename PartitionedHypergraph::template DeltaPartition<DeltaGainCache::requires_connectivity_set>;
using AttributedGains = typename GainTypes::AttributedGains;
using AttributedGains = typename CombinedTraits::AttributedGains;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think AttributedGains can be removed.

@larsgottesbueren larsgottesbueren self-requested a review October 4, 2023 09:43
N-Maas and others added 23 commits October 4, 2023 14:19
~7% overall speedup with all features
~14% overall speedup with all features
…gorithms

This splits the long compile time of pool_initial_partitioner in two
…lass

reduces compile time of register_initial_partitioning_algorithms
significantly
@N-Maas N-Maas merged commit aa1dbb0 into master Oct 4, 2023
@N-Maas N-Maas deleted the compiletimes branch November 30, 2023 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants